First phase/step of the auth flow implementation. Also collapse the r… #343
Conversation
…etry behaviors in to the same thing. We perform the docker style auth flow (used by all service providers it seems), right now however we are not including support for the credentials helpers to lookup username/password combos. But this is enough to now authenticate against docker hub anonymously as it requires to fetch anything
|
I believe it should be a case of shelling out to the auth helpers of docker to enable full auth capabilities, but if this seems ok i'll try add that next since need to connect to azure's ones authenticated too |
src/registry/http/blob.rs
Outdated
| let mut r = self | ||
| .http_client | ||
| .request( | ||
| &uri, | ||
| (), | ||
| |_, c| async { | ||
| c.method(http::Method::HEAD) | ||
| .body(Body::from("")) | ||
| .map_err(|e| e.into()) | ||
| }, | ||
| 3, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
minor nit: Could we try to refactor this block of sending blank body into a function that takes an http method?
There was a problem hiding this comment.
I added a request_simple method helper and moved these to use that if i understood the request right?
src/registry/http/blob.rs
Outdated
| let stream = futures::stream::unfold( | ||
| (context.progress_bar.clone(), ReaderStream::new(f), 0), | ||
| |(progress_bar_cp, mut reader_stream, read_bytes)| async move { | ||
| let nxt_chunk = if let Some(c) = reader_stream.next().await { |
There was a problem hiding this comment.
couldn't we do let next_chunk = reader_stream.next().await? here?
There was a problem hiding this comment.
Yup, i think the type infernce was mad at me when writing it. But its happy with this
| let mut response = run_single_request( | ||
| Default::default(), | ||
| &new_uri, | ||
| basic_auth_info, |
There was a problem hiding this comment.
I'm confused, it looks like the basic_auth_info is always None, so below the match on basic_auth_info will always fail. Am I missing something?
Could we comment to help the next reader, or remove the pattern match on basic_auth_info if I'm right?
There was a problem hiding this comment.
Added comment indicating its a TODO. Can also remove it until the rest lands if you'd rather though.
src/registry/http/http_cli/mod.rs
Outdated
| pub struct HttpCli { | ||
| pub inner_client: Client<hyper_rustls::HttpsConnector<hyper::client::HttpConnector>>, | ||
| pub credentials: Arc<tokio::sync::Mutex<Option<bool>>>, | ||
| pub auth_info: Arc<tokio::sync::Mutex<Option<AuthResponse>>>, |
There was a problem hiding this comment.
nit: can we import tokio::sync::Mutex rather than duplicate?
|
Updated for all the comments I think. Thanks! |
…etry behaviors in to the same thing. We perform the docker style auth flow (used by all service providers it seems), right now however we are not including support for the credentials helpers to lookup username/password combos. But this is enough to now authenticate against docker hub anonymously as it requires to fetch anything
This does make some of the request semantics a bit more complex in the simplest cases as we are handling the retries, redirects, authentication in the one place.
There is an extra parameter context as it seemed like the best way to be able to make state from a wrapping method into the passed async lambda that might be called many times.
Tested this manually doing queries from docker hub and it worked.